Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(toolchain): bump to 2022-10-16 #5119

Closed
wants to merge 18 commits into from
Closed

build(toolchain): bump to 2022-10-16 #5119

wants to merge 18 commits into from

Conversation

xxchan
Copy link
Member

@xxchan xxchan commented Sep 5, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

@xxchan
Copy link
Member Author

xxchan commented Sep 13, 2022

The ICE is fixed. Now we have TAIT bug 🤪 rust-lang/rust#101750

@xxchan xxchan changed the title build(toolchain): bump to 2022-09-05 build(toolchain): bump to 2022-09-13 Sep 13, 2022
@xxchan xxchan changed the title build(toolchain): bump to 2022-09-13 build(toolchain): bump to 2022-09-22 Sep 22, 2022
@BugenZhao
Copy link
Member

Really want rust-lang/cargo#11114 after bumping. 🤤

@xxchan xxchan marked this pull request as ready for review October 16, 2022 22:10
@xxchan xxchan changed the title build(toolchain): bump to 2022-09-22 build(toolchain): bump to 2022-10-16 Oct 16, 2022
src/storage/src/lib.rs Show resolved Hide resolved
Comment on lines +15 to +17
#![expect(clippy::iter_kv_map, reason = "FIXME: fix later")]
#![expect(
clippy::or_fun_call,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

.ok_or(ValueEncodingError::InvalidNaiveTimeEncoding(secs, nano))?,
.ok_or_else(|| ValueEncodingError::InvalidNaiveTimeEncoding(secs, nano))?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be any difference for enum construction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skyzh
Copy link
Contributor

skyzh commented Oct 17, 2022

Build of risingwave_stream will stuck on macOS. Still investigating why.

@skyzh
Copy link
Contributor

skyzh commented Oct 17, 2022

INFO rustc_trait_selection::traits::query::normalize normalize::<rustc_middle::mir::ConstantKind>: result=Ok(Val(ZeroSized, for<'a, 'b, 'c> fn(std::pin::Pin<&'a mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>>, &'b mut std::task::Context<'c>) -> std::task::Poll<std::option::Option<<futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]> as tokio_stream::Stream>::Item>> {<futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]> as tokio_stream::Stream>::poll_next})) with 4052 obligations
INFO rustc_trait_selection::traits::query::normalize normalize::<rustc_middle::ty::subst::GenericArg>: result=Ok(unsafe fn(&mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>) -> std::pin::Pin<&mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>> {std::pin::Pin::<&mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>>::new_unchecked}) with 4052 obligations
INFO rustc_trait_selection::traits::query::normalize normalize::<rustc_middle::mir::ConstantKind>: result=Ok(Val(ZeroSized, unsafe fn(&mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>) -> std::pin::Pin<&mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>> {std::pin::Pin::<&mut futures_async_stream::try_stream::from_generator::GenTryStream<[static generator@src/stream/src/executor/hash_join.rs:760:5: 760:61]>>::new_unchecked})) with 4052 obligations

It seems that Rust is very slow with resolving futures_async_stream. It took about 1sec for each resolving.

@skyzh
Copy link
Contributor

skyzh commented Oct 17, 2022

To reproduce, RUSTC_LOG=debug ./risedev d

@yuhao-su
Copy link
Contributor

Do you have time to look into why HashJoin is causing rustc to stuck?

I'll take a look.

@skyzh
Copy link
Contributor

skyzh commented Oct 18, 2022

Unluckily, the bombing obligations seem to originate from StateTableIter

INFO rustc_trait_selection::traits::query::normalize normalize::<rustc_middle::ty::subst::GenericArg>: result=Ok(std::future::from_generator::GenFuture<[static generator@src/stream/src/common/mod.rs:31:45: 37:2]>) with 4052 obligations
INFO rustc_trait_selection::traits::query::normalize normalize::<rustc_middle::ty::subst::GenericArg>: result=Ok(std::future::from_generator::GenFuture<[static generator@risingwave_storage::table::streaming_table::state_table::StateTable<risingwave_storage::monitor::MonitoredStateStore<risingwave_storage::hummock::HummockStorage>>::iter::{closure#0}]>) with 4052 obligations
INFO rustc_trait_selection::traits::query::normalize normalize::<rustc_middle::ty::subst::GenericArg>: result=Ok(std::future::from_generator::GenFuture<[static generator@risingwave_storage::table::streaming_table::state_table::StateTable<risingwave_storage::monitor::MonitoredStateStore<risingwave_storage::hummock::HummockStorage>>::iter_with_pk_prefix::{closure#0}]>) with 4052 obligations

which cases all executors to have a lot of obligations, whereas hash join needs a lot of specialization, and cases the most problems.

@skyzh
Copy link
Contributor

skyzh commented Oct 18, 2022

After changing iter_key_and_val to boxed, risingwave_stream compiles in 40 seconds (still seems slow). cc @BugenZhao now it's your turn to investigate what's happening 🥵

    pub async fn iter_key_and_val<'a>(
        &'a self,
        pk_prefix: &'a Row,
    ) -> StorageResult<RowStreamWithPk<'a, S>> {
        let (mem_table_iter, storage_iter_stream) = self
            .iter_with_pk_prefix_inner(pk_prefix, self.epoch())
            .await?;
        let storage_iter = storage_iter_stream.into_stream().boxed();

        Ok(
            StateTableRowIter::new(mem_table_iter, storage_iter, self.row_deserializer.clone())
                .into_stream().boxed(),
        )
    }

@skyzh
Copy link
Contributor

skyzh commented Oct 18, 2022

My wild guess is that while rustc allows more lifetime generics in the latest version, some wrongly-labeled lifetimes will cause compile issues. Every time we add 'a at &self, there must be something going wrong.

@skyzh
Copy link
Contributor

skyzh commented Oct 18, 2022

The issue now narrows down to risingwave_storage. It seems that it produces 4052 obligations...

image

Most of them are OutlivesPredicate (lifetime issues)

@TennyZhuang
Copy link
Contributor

turn to @wenym1

@TennyZhuang
Copy link
Contributor

IIRC storage have many unnecessary generics, but I’m not sure whether it’s related.

@skyzh
Copy link
Contributor

skyzh commented Oct 18, 2022

I feel this issue more related to StateTable implementation than HummockStorage itself. The obligations will only bomb when processing StateTable-related code.

@BugenZhao
Copy link
Member

Will implementing state table iterator manually with poll_next help?

@skyzh
Copy link
Contributor

skyzh commented Oct 18, 2022

I'm not sure, but I'd suggest investigating where's the obligations from.

@MrCroxx
Copy link
Contributor

MrCroxx commented Oct 19, 2022

Also the lifetime issue doesn't look like a false positive.

        let rotate_last_mut = |buffers: &'a mut Vec<_>| {
            buffers.push(DioBuffer::with_capacity_in(
                self.buffer_capacity,
                &DIO_BUFFER_ALLOCATOR,
            ));
            buffers.last_mut().unwrap()
        };

is defined for 'a mut Vec

where

rotate_last_mut(&mut self.buffers),

is following the lifetime of &mut self. buffers' lifetime has no relationship to 'a. The compiler's report seems correct.

cc @MrCroxx would you please help fix this?

Sure. I'll handle it.

@lmatz
Copy link
Contributor

lmatz commented Oct 24, 2022

#1334 can be closed after this

@skyzh
Copy link
Contributor

skyzh commented Oct 25, 2022

I'll take over this PR and get it merged with my pace.

@victoryang00
Copy link

Thank you guys very much

@skyzh
Copy link
Contributor

skyzh commented Oct 27, 2022

close in favor of #6025

@skyzh skyzh closed this Oct 27, 2022
@xxchan xxchan deleted the xxchan/bump branch November 4, 2022 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants